[clang]: Propagate `*noreturn` attributes in `CFG` #146355

negativ · 2025-06-30T14:12:15Z

Summary

This PR extends Clang's CFG analysis to automatically propagate analyzer_noreturn attributes through function call chains. When a function exclusively calls no-return functions, it is automatically treated as no-return as well, improving static analysis precision.

Problem

Currently, simple forwarder functions to no-return calls are not recognized as no-return themselves:

void terminate() __attribute__((analyzer_noreturn)) {  }

void fatal_error() {
    terminate();  // This "never" returns, but fatal_error() is not marked as no-return
}

void handle_error(const std::optional<int> opt) {
    if (!opt) {
        fatal_error(); // Static analyzer doesn't know this never returns
    }
    *opt = 1;      // False positive: analyzer thinks this is reachable
}

Solution

Enhanced isImmediateSinkBlock() in CFG.cpp to perform inter-procedural analysis:

Existing behavior preserved: Direct no-return elements and throw expressions
Enhanced function call analysis:
- Checks for existing no-return attributes (analyzer_noreturn, noreturn, [[noreturn]], _Noreturn)
- Recognizes well-known C library functions (exit, abort, __assert_fail, etc.)
- Supports namespaced functions (BloombergLP::bsls::Assert::invokeHandler, std::terminate)
Recursive CFG analysis: For user-defined functions, builds CFG and checks if all execution paths lead to no-return calls

Implementation Details

Modified isImmediateSinkBlock() in lib/Analysis/CFG.cpp
Added support for 10+ well-known no-return C and C++ functions
Inter-procedural analysis limited to functions with available bodies

github-actions · 2025-06-30T14:12:34Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

MikeWeller

Quick pass / haven't looked in-depth yet.

I would mention in the description/summary that plain noreturn already has this behaviour (at least in one of the CFG.cpp vs. NoReturnFunctionChecker.cpp implementations) so this is adding the same behaviour for analyzer_noreturn.

MikeWeller · 2025-07-01T10:20:59Z

clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp

+      assertion_handler();
+  }
+
+  *opt; // no-warning


Is no-warning handled by the test harness? I'm familiar/have seen // CHECK-NOT: warning comments. Unless any extraneous warnings already fail the test.

It's just a comment that we do not expect warning here. check_clang_tidy.py ignores it.

Anyway, i consider moving this kind of check to clang\unittests\Analysis\FlowSensitive\UncheckedOptionalAccessModelTest.cpp

OK, I was just checking/asking whether the test will fail if a warning is emitted or whether you need a // CHECK-NOT: warning. But it seems for most tests any warning will produce a failure.

clang-tidy tests require an exact match of diagnostics, so too many or too few diagnostics will fail the test

MikeWeller · 2025-07-01T10:23:53Z

clang/lib/Analysis/CFG.cpp

+    if (!NoReturn)
+      NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context);
+
+    // Some well-known 'noreturn' functions


Can we consolidate this list with the one in NoReturnFunctionChecker.cpp? Or at least reference the other one to remind people to update both.

@MikeWeller i've added comment in NoreturnFunctionChecker.cpp

llvmbot · 2025-07-07T09:57:50Z

@llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-analysis

Author: Andrey Karlov (negativ)

Changes

Summary

This PR extends Clang's CFG analysis to automatically propagate analyzer_noreturn attributes through function call chains. When a function exclusively calls no-return functions, it is automatically treated as no-return as well, improving static analysis precision.

Problem

Currently, simple forwarder functions to no-return calls are not recognized as no-return themselves:

void terminate() __attribute__((analyzer_noreturn)) {  }

void fatal_error() {
    terminate();  // This "never" returns, but fatal_error() is not marked as no-return
}

void handle_error(const std::optional&lt;int&gt; opt) {
    if (!opt) {
        fatal_error(); // Static analyzer doesn't know this never returns
    }
    *opt = 1;      // False positive: analyzer thinks this is reachable
}

Solution

Enhanced isImmediateSinkBlock() in CFG.cpp to perform inter-procedural analysis:

Existing behavior preserved: Direct no-return elements and throw expressions
Enhanced function call analysis:
- Checks for existing no-return attributes (analyzer_noreturn, noreturn, [[noreturn]], _Noreturn)
- Recognizes well-known C library functions (exit, abort, __assert_fail, etc.)
- Supports namespaced functions (BloombergLP::bsls::Assert::invokeHandler, std::terminate)
Recursive CFG analysis: For user-defined functions, builds CFG and checks if all execution paths lead to no-return calls

Implementation Details

Modified isImmediateSinkBlock() in lib/Analysis/CFG.cpp
Added support for 10+ well-known no-return C and C++ functions
Inter-procedural analysis limited to functions with available bodies

Full diff: https://github.com/llvm/llvm-project/pull/146355.diff

7 Files Affected:

(modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp (+36)
(modified) clang/include/clang/AST/Decl.h (+5)
(modified) clang/lib/AST/Decl.cpp (+4)
(modified) clang/lib/Analysis/CFG.cpp (+74-5)
(modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+2-2)
(modified) clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp (+39-31)
(modified) clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp (+81)

diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..f4f82199a0bb5 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,42 @@ void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, Bloo
   }
 }
 
+void assertion_handler_imp() __attribute__((analyzer_noreturn));
+
+void assertion_handler() {
+    do {
+       assertion_handler_imp();
+    } while(0);
+}
+
+void function_calling_analyzer_noreturn(const bsl::optional<int>& opt)
+{
+  if (!opt) {
+      assertion_handler();
+  }
+
+  *opt; // no-warning
+}
+
+void abort();
+
+void do_fail() {
+    abort(); // acts like 'abort()' C-function
+}
+
+void invoke_assertion_handler() {
+    do_fail();
+}
+
+void function_calling_well_known_noreturn(const bsl::optional<int>& opt)
+{
+  if (!opt) {
+      invoke_assertion_handler();
+  }
+
+  *opt; // no-warning
+}
+
 template <typename T>
 void function_template_without_user(const absl::optional<T> &opt) {
   opt.value(); // no-warning
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index f70a039bf3517..6ada8785fd0ba 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -2630,6 +2630,11 @@ class FunctionDecl : public DeclaratorDecl,
   /// an attribute on its declaration or its type.
   bool isNoReturn() const;
 
+  /// Determines whether this function is known to never return for CFG
+  /// analysis. Checks for noreturn attributes on the function declaration
+  /// or its type, including 'analyzer_noreturn' attribute.
+  bool isAnalyzerNoReturn() const;
+
   /// True if the function was a definition but its body was skipped.
   bool hasSkippedBody() const { return FunctionDeclBits.HasSkippedBody; }
   void setHasSkippedBody(bool Skipped = true) {
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index e0362245d6ecd..19336a8e942cd 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3595,6 +3595,10 @@ bool FunctionDecl::isNoReturn() const {
   return false;
 }
 
+bool FunctionDecl::isAnalyzerNoReturn() const {
+  return isNoReturn() || hasAttr<AnalyzerNoReturnAttr>();
+}
+
 bool FunctionDecl::isMemberLikeConstrainedFriend() const {
   // C++20 [temp.friend]p9:
   //   A non-template friend declaration with a requires-clause [or]
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index d960d5130332b..bc47891216e7a 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -41,6 +41,7 @@
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -2833,8 +2834,37 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) {
     if (!FD->isVariadic())
       findConstructionContextsForArguments(C);
 
-    if (FD->isNoReturn() || C->isBuiltinAssumeFalse(*Context))
-      NoReturn = true;
+    if (!NoReturn)
+      NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context);
+
+    // Some well-known 'noreturn' functions
+    if (!NoReturn)
+      NoReturn = llvm::StringSwitch<bool>(FD->getQualifiedNameAsString())
+                     .Case("BloombergLP::bsls::Assert::invokeHandler", true)
+                     .Case("std::terminate", true)
+                     .Case("std::abort", true)
+                     .Case("exit", true)
+                     .Case("abort", true)
+                     .Case("panic", true)
+                     .Case("error", true)
+                     .Case("Assert", true)
+                     .Case("ziperr", true)
+                     .Case("assfail", true)
+                     .Case("db_error", true)
+                     .Case("__assert", true)
+                     .Case("__assert2", true)
+                     .Case("_wassert", true)
+                     .Case("__assert_rtn", true)
+                     .Case("__assert_fail", true)
+                     .Case("dtrace_assfail", true)
+                     .Case("yy_fatal_error", true)
+                     .Case("_XCAssertionFailureHandler", true)
+                     .Case("_DTAssertionFailureHandler", true)
+                     .Case("_TSAssertionFailureHandler", true)
+                     .Case("__builtin_trap", true)
+                     .Case("__builtin_unreachable", true)
+                     .Default(false);
+
     if (FD->hasAttr<NoThrowAttr>())
       AddEHEdge = false;
     if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) ||
@@ -6288,6 +6318,12 @@ void CFGBlock::printTerminatorJson(raw_ostream &Out, const LangOptions &LO,
 // There may be many more reasons why a sink would appear during analysis
 // (eg. checkers may generate sinks arbitrarily), but here we only consider
 // sinks that would be obvious by looking at the CFG.
+//
+// This function also performs inter-procedural analysis by recursively
+// examining called functions to detect forwarding chains to noreturn
+// functions. When a function is determined to never return through this
+// analysis, it's automatically marked with analyzer_noreturn attribute
+// for caching and future reference.
 static bool isImmediateSinkBlock(const CFGBlock *Blk) {
   if (Blk->hasNoReturnElement())
     return true;
@@ -6298,10 +6334,43 @@ static bool isImmediateSinkBlock(const CFGBlock *Blk) {
   // at least for now, but once we have better support for exceptions,
   // we'd need to carefully handle the case when the throw is being
   // immediately caught.
-  if (llvm::any_of(*Blk, [](const CFGElement &Elm) {
+  if (llvm::any_of(*Blk, [](const CFGElement &Elm) -> bool {
+        if (std::optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
+          return isa<CXXThrowExpr>(StmtElm->getStmt());
+        return false;
+      }))
+    return true;
+
+  auto HasNoReturnCall = [](const CallExpr *CE) {
+    if (!CE)
+      return false;
+
+    static thread_local llvm::SmallPtrSet<const FunctionDecl *, 32> InProgress;
+
+    auto *FD = CE->getDirectCallee();
+
+    if (!FD || InProgress.count(FD))
+      return false;
+
+    InProgress.insert(FD);
+    auto DoCleanup = llvm::make_scope_exit([&]() { InProgress.erase(FD); });
+
+    auto NoReturnFromCFG = [FD]() {
+      if (!FD->getBody())
+        return false;
+
+      auto CalleeCFG =
+          CFG::buildCFG(FD, FD->getBody(), &FD->getASTContext(), {});
+
+      return CalleeCFG && CalleeCFG->getEntry().isInevitablySinking();
+    };
+
+    return FD->isAnalyzerNoReturn() || NoReturnFromCFG();
+  };
+
+  if (llvm::any_of(*Blk, [&](const CFGElement &Elm) {
         if (std::optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
-          if (isa<CXXThrowExpr>(StmtElm->getStmt()))
-            return true;
+          return HasNoReturnCall(dyn_cast<CallExpr>(StmtElm->getStmt()));
         return false;
       }))
     return true;
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 1113bbe7f4d9c..c799ca98e4a0d 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -283,7 +283,7 @@ computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) {
   JoinedStateBuilder Builder(AC, JoinBehavior);
   for (const CFGBlock *Pred : Preds) {
     // Skip if the `Block` is unreachable or control flow cannot get past it.
-    if (!Pred || Pred->hasNoReturnElement())
+    if (!Pred || Pred->isInevitablySinking())
       continue;
 
     // Skip if `Pred` was not evaluated yet. This could happen if `Pred` has a
@@ -562,7 +562,7 @@ runTypeErasedDataflowAnalysis(
     BlockStates[Block->getBlockID()] = std::move(NewBlockState);
 
     // Do not add unreachable successor blocks to `Worklist`.
-    if (Block->hasNoReturnElement())
+    if (Block->isInevitablySinking())
       continue;
 
     Worklist.enqueueSuccessors(Block);
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
index 17c3cb4e9e04c..834bd81c2fa21 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
@@ -50,37 +50,45 @@ void NoReturnFunctionChecker::checkPostCall(const CallEvent &CE,
       BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn();
   }
 
-  if (!BuildSinks && CE.isGlobalCFunction()) {
-    if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
-      // HACK: Some functions are not marked noreturn, and don't return.
-      //  Here are a few hardwired ones.  If this takes too long, we can
-      //  potentially cache these results.
-      BuildSinks
-        = llvm::StringSwitch<bool>(StringRef(II->getName()))
-            .Case("exit", true)
-            .Case("panic", true)
-            .Case("error", true)
-            .Case("Assert", true)
-            // FIXME: This is just a wrapper around throwing an exception.
-            //  Eventually inter-procedural analysis should handle this easily.
-            .Case("ziperr", true)
-            .Case("assfail", true)
-            .Case("db_error", true)
-            .Case("__assert", true)
-            .Case("__assert2", true)
-            // For the purpose of static analysis, we do not care that
-            //  this MSVC function will return if the user decides to continue.
-            .Case("_wassert", true)
-            .Case("__assert_rtn", true)
-            .Case("__assert_fail", true)
-            .Case("dtrace_assfail", true)
-            .Case("yy_fatal_error", true)
-            .Case("_XCAssertionFailureHandler", true)
-            .Case("_DTAssertionFailureHandler", true)
-            .Case("_TSAssertionFailureHandler", true)
-            .Default(false);
-    }
-  }
+ if (!BuildSinks && CE.isGlobalCFunction()) {
+   if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
+     // HACK: Some functions are not marked noreturn, and don't return.
+     //  Here are a few hardwired ones.  If this takes too long, we can
+     //  potentially cache these results.
+     //
+     // (!) In case of function list update, please also update
+     // CFGBuilder::VisitCallExpr (CFG.cpp)
+     BuildSinks =
+         llvm::StringSwitch<bool>(StringRef(II->getName()))
+             .Case("exit", true)
+             .Case("abort", true)
+             .Case("panic", true)
+             .Case("error", true)
+             .Case("Assert", true)
+             // FIXME: This is just a wrapper around throwing an exception.
+             //  Eventually inter-procedural analysis should handle this
+             //  easily.
+             .Case("ziperr", true)
+             .Case("assfail", true)
+             .Case("db_error", true)
+             .Case("__assert", true)
+             .Case("__assert2", true)
+             // For the purpose of static analysis, we do not care that
+             //  this MSVC function will return if the user decides to
+             //  continue.
+             .Case("_wassert", true)
+             .Case("__assert_rtn", true)
+             .Case("__assert_fail", true)
+             .Case("dtrace_assfail", true)
+             .Case("yy_fatal_error", true)
+             .Case("_XCAssertionFailureHandler", true)
+             .Case("_DTAssertionFailureHandler", true)
+             .Case("_TSAssertionFailureHandler", true)
+             .Case("__builtin_trap", true)
+             .Case("__builtin_unreachable", true)
+             .Default(false);
+   }
+ }
 
   if (BuildSinks)
     C.generateSink(C.getState(), C.getPredecessor());
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 9fb7bebdbe41e..9150b3e155533 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -49,6 +49,7 @@ using namespace ast_matchers;
 using llvm::IsStringMapEntry;
 using ::testing::DescribeMatcher;
 using ::testing::IsEmpty;
+using ::testing::Not;
 using ::testing::NotNull;
 using ::testing::Test;
 using ::testing::UnorderedElementsAre;
@@ -693,6 +694,86 @@ TEST_F(NoreturnDestructorTest, ConditionalOperatorNestedBranchReturns) {
   // FIXME: Called functions at point `p` should contain only "foo".
 }
 
+class AnalyzerNoreturnTest : public Test {
+protected:
+  template <typename Matcher>
+  void runDataflow(llvm::StringRef Code, Matcher Expectations) {
+    tooling::FileContentMappings FilesContents;
+    FilesContents.push_back(
+        std::make_pair<std::string, std::string>("noreturn_test_defs.h", R"(
+      void assertionHandler() __attribute__((analyzer_noreturn));
+
+      void assertionTrampoline() {
+        assertionHandler();
+      }
+
+      void trap() {}
+    )"));
+
+    ASSERT_THAT_ERROR(
+        test::checkDataflow<FunctionCallAnalysis>(
+            AnalysisInputs<FunctionCallAnalysis>(
+                Code, ast_matchers::hasName("target"),
+                [](ASTContext &C, Environment &) {
+                  return FunctionCallAnalysis(C);
+                })
+                .withASTBuildArgs({"-fsyntax-only", "-std=c++17"})
+                .withASTBuildVirtualMappedFiles(std::move(FilesContents)),
+            /*VerifyResults=*/
+            [&Expectations](
+                const llvm::StringMap<
+                    DataflowAnalysisState<FunctionCallLattice>> &Results,
+                const AnalysisOutputs &) {
+              EXPECT_THAT(Results, Expectations);
+            }),
+        llvm::Succeeded());
+  }
+};
+
+TEST_F(AnalyzerNoreturnTest, Breathing) {
+  std::string Code = R"(
+    #include "noreturn_test_defs.h"
+
+    void target() {
+      trap();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, UnorderedElementsAre(IsStringMapEntry(
+                        "p", HoldsFunctionCallLattice(HasCalledFunctions(
+                                 UnorderedElementsAre("trap"))))));
+}
+
+TEST_F(AnalyzerNoreturnTest, DirectNoReturnCall) {
+  std::string Code = R"(
+    #include "noreturn_test_defs.h"
+
+    void target() {
+      assertionHandler();
+      trap();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry(
+                        "p", HoldsFunctionCallLattice(HasCalledFunctions(
+                                 UnorderedElementsAre("trap")))))));
+}
+
+TEST_F(AnalyzerNoreturnTest, IndirectNoReturnCall) {
+  std::string Code = R"(
+    #include "noreturn_test_defs.h"
+
+    void target() {
+      assertionTrampoline();
+      trap();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry(
+                        "p", HoldsFunctionCallLattice(HasCalledFunctions(
+                                 UnorderedElementsAre("trap")))))));
+}
+
 // Models an analysis that uses flow conditions.
 class SpecialBoolAnalysis final
     : public DataflowAnalysis<SpecialBoolAnalysis, NoopLattice> {

llvmbot · 2025-07-07T09:57:50Z

@llvm/pr-subscribers-clang-tidy

Author: Andrey Karlov (negativ)

Changes

Summary

This PR extends Clang's CFG analysis to automatically propagate analyzer_noreturn attributes through function call chains. When a function exclusively calls no-return functions, it is automatically treated as no-return as well, improving static analysis precision.

Problem

Currently, simple forwarder functions to no-return calls are not recognized as no-return themselves:

void terminate() __attribute__((analyzer_noreturn)) {  }

void fatal_error() {
    terminate();  // This "never" returns, but fatal_error() is not marked as no-return
}

void handle_error(const std::optional&lt;int&gt; opt) {
    if (!opt) {
        fatal_error(); // Static analyzer doesn't know this never returns
    }
    *opt = 1;      // False positive: analyzer thinks this is reachable
}

Solution

Enhanced isImmediateSinkBlock() in CFG.cpp to perform inter-procedural analysis:

Existing behavior preserved: Direct no-return elements and throw expressions
Enhanced function call analysis:
- Checks for existing no-return attributes (analyzer_noreturn, noreturn, [[noreturn]], _Noreturn)
- Recognizes well-known C library functions (exit, abort, __assert_fail, etc.)
- Supports namespaced functions (BloombergLP::bsls::Assert::invokeHandler, std::terminate)
Recursive CFG analysis: For user-defined functions, builds CFG and checks if all execution paths lead to no-return calls

Implementation Details

Modified isImmediateSinkBlock() in lib/Analysis/CFG.cpp
Added support for 10+ well-known no-return C and C++ functions
Inter-procedural analysis limited to functions with available bodies

Full diff: https://github.com/llvm/llvm-project/pull/146355.diff

7 Files Affected:

(modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp (+36)
(modified) clang/include/clang/AST/Decl.h (+5)
(modified) clang/lib/AST/Decl.cpp (+4)
(modified) clang/lib/Analysis/CFG.cpp (+74-5)
(modified) clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp (+2-2)
(modified) clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp (+39-31)
(modified) clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp (+81)

diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
index 3167b85f0e024..f4f82199a0bb5 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp
@@ -141,6 +141,42 @@ void nullable_value_after_swap(BloombergLP::bdlb::NullableValue<int> &opt1, Bloo
   }
 }
 
+void assertion_handler_imp() __attribute__((analyzer_noreturn));
+
+void assertion_handler() {
+    do {
+       assertion_handler_imp();
+    } while(0);
+}
+
+void function_calling_analyzer_noreturn(const bsl::optional<int>& opt)
+{
+  if (!opt) {
+      assertion_handler();
+  }
+
+  *opt; // no-warning
+}
+
+void abort();
+
+void do_fail() {
+    abort(); // acts like 'abort()' C-function
+}
+
+void invoke_assertion_handler() {
+    do_fail();
+}
+
+void function_calling_well_known_noreturn(const bsl::optional<int>& opt)
+{
+  if (!opt) {
+      invoke_assertion_handler();
+  }
+
+  *opt; // no-warning
+}
+
 template <typename T>
 void function_template_without_user(const absl::optional<T> &opt) {
   opt.value(); // no-warning
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index f70a039bf3517..6ada8785fd0ba 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -2630,6 +2630,11 @@ class FunctionDecl : public DeclaratorDecl,
   /// an attribute on its declaration or its type.
   bool isNoReturn() const;
 
+  /// Determines whether this function is known to never return for CFG
+  /// analysis. Checks for noreturn attributes on the function declaration
+  /// or its type, including 'analyzer_noreturn' attribute.
+  bool isAnalyzerNoReturn() const;
+
   /// True if the function was a definition but its body was skipped.
   bool hasSkippedBody() const { return FunctionDeclBits.HasSkippedBody; }
   void setHasSkippedBody(bool Skipped = true) {
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index e0362245d6ecd..19336a8e942cd 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3595,6 +3595,10 @@ bool FunctionDecl::isNoReturn() const {
   return false;
 }
 
+bool FunctionDecl::isAnalyzerNoReturn() const {
+  return isNoReturn() || hasAttr<AnalyzerNoReturnAttr>();
+}
+
 bool FunctionDecl::isMemberLikeConstrainedFriend() const {
   // C++20 [temp.friend]p9:
   //   A non-template friend declaration with a requires-clause [or]
diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index d960d5130332b..bc47891216e7a 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -41,6 +41,7 @@
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
@@ -2833,8 +2834,37 @@ CFGBlock *CFGBuilder::VisitCallExpr(CallExpr *C, AddStmtChoice asc) {
     if (!FD->isVariadic())
       findConstructionContextsForArguments(C);
 
-    if (FD->isNoReturn() || C->isBuiltinAssumeFalse(*Context))
-      NoReturn = true;
+    if (!NoReturn)
+      NoReturn = FD->isAnalyzerNoReturn() || C->isBuiltinAssumeFalse(*Context);
+
+    // Some well-known 'noreturn' functions
+    if (!NoReturn)
+      NoReturn = llvm::StringSwitch<bool>(FD->getQualifiedNameAsString())
+                     .Case("BloombergLP::bsls::Assert::invokeHandler", true)
+                     .Case("std::terminate", true)
+                     .Case("std::abort", true)
+                     .Case("exit", true)
+                     .Case("abort", true)
+                     .Case("panic", true)
+                     .Case("error", true)
+                     .Case("Assert", true)
+                     .Case("ziperr", true)
+                     .Case("assfail", true)
+                     .Case("db_error", true)
+                     .Case("__assert", true)
+                     .Case("__assert2", true)
+                     .Case("_wassert", true)
+                     .Case("__assert_rtn", true)
+                     .Case("__assert_fail", true)
+                     .Case("dtrace_assfail", true)
+                     .Case("yy_fatal_error", true)
+                     .Case("_XCAssertionFailureHandler", true)
+                     .Case("_DTAssertionFailureHandler", true)
+                     .Case("_TSAssertionFailureHandler", true)
+                     .Case("__builtin_trap", true)
+                     .Case("__builtin_unreachable", true)
+                     .Default(false);
+
     if (FD->hasAttr<NoThrowAttr>())
       AddEHEdge = false;
     if (isBuiltinAssumeWithSideEffects(FD->getASTContext(), C) ||
@@ -6288,6 +6318,12 @@ void CFGBlock::printTerminatorJson(raw_ostream &Out, const LangOptions &LO,
 // There may be many more reasons why a sink would appear during analysis
 // (eg. checkers may generate sinks arbitrarily), but here we only consider
 // sinks that would be obvious by looking at the CFG.
+//
+// This function also performs inter-procedural analysis by recursively
+// examining called functions to detect forwarding chains to noreturn
+// functions. When a function is determined to never return through this
+// analysis, it's automatically marked with analyzer_noreturn attribute
+// for caching and future reference.
 static bool isImmediateSinkBlock(const CFGBlock *Blk) {
   if (Blk->hasNoReturnElement())
     return true;
@@ -6298,10 +6334,43 @@ static bool isImmediateSinkBlock(const CFGBlock *Blk) {
   // at least for now, but once we have better support for exceptions,
   // we'd need to carefully handle the case when the throw is being
   // immediately caught.
-  if (llvm::any_of(*Blk, [](const CFGElement &Elm) {
+  if (llvm::any_of(*Blk, [](const CFGElement &Elm) -> bool {
+        if (std::optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
+          return isa<CXXThrowExpr>(StmtElm->getStmt());
+        return false;
+      }))
+    return true;
+
+  auto HasNoReturnCall = [](const CallExpr *CE) {
+    if (!CE)
+      return false;
+
+    static thread_local llvm::SmallPtrSet<const FunctionDecl *, 32> InProgress;
+
+    auto *FD = CE->getDirectCallee();
+
+    if (!FD || InProgress.count(FD))
+      return false;
+
+    InProgress.insert(FD);
+    auto DoCleanup = llvm::make_scope_exit([&]() { InProgress.erase(FD); });
+
+    auto NoReturnFromCFG = [FD]() {
+      if (!FD->getBody())
+        return false;
+
+      auto CalleeCFG =
+          CFG::buildCFG(FD, FD->getBody(), &FD->getASTContext(), {});
+
+      return CalleeCFG && CalleeCFG->getEntry().isInevitablySinking();
+    };
+
+    return FD->isAnalyzerNoReturn() || NoReturnFromCFG();
+  };
+
+  if (llvm::any_of(*Blk, [&](const CFGElement &Elm) {
         if (std::optional<CFGStmt> StmtElm = Elm.getAs<CFGStmt>())
-          if (isa<CXXThrowExpr>(StmtElm->getStmt()))
-            return true;
+          return HasNoReturnCall(dyn_cast<CallExpr>(StmtElm->getStmt()));
         return false;
       }))
     return true;
diff --git a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
index 1113bbe7f4d9c..c799ca98e4a0d 100644
--- a/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
+++ b/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp
@@ -283,7 +283,7 @@ computeBlockInputState(const CFGBlock &Block, AnalysisContext &AC) {
   JoinedStateBuilder Builder(AC, JoinBehavior);
   for (const CFGBlock *Pred : Preds) {
     // Skip if the `Block` is unreachable or control flow cannot get past it.
-    if (!Pred || Pred->hasNoReturnElement())
+    if (!Pred || Pred->isInevitablySinking())
       continue;
 
     // Skip if `Pred` was not evaluated yet. This could happen if `Pred` has a
@@ -562,7 +562,7 @@ runTypeErasedDataflowAnalysis(
     BlockStates[Block->getBlockID()] = std::move(NewBlockState);
 
     // Do not add unreachable successor blocks to `Worklist`.
-    if (Block->hasNoReturnElement())
+    if (Block->isInevitablySinking())
       continue;
 
     Worklist.enqueueSuccessors(Block);
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
index 17c3cb4e9e04c..834bd81c2fa21 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
@@ -50,37 +50,45 @@ void NoReturnFunctionChecker::checkPostCall(const CallEvent &CE,
       BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn();
   }
 
-  if (!BuildSinks && CE.isGlobalCFunction()) {
-    if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
-      // HACK: Some functions are not marked noreturn, and don't return.
-      //  Here are a few hardwired ones.  If this takes too long, we can
-      //  potentially cache these results.
-      BuildSinks
-        = llvm::StringSwitch<bool>(StringRef(II->getName()))
-            .Case("exit", true)
-            .Case("panic", true)
-            .Case("error", true)
-            .Case("Assert", true)
-            // FIXME: This is just a wrapper around throwing an exception.
-            //  Eventually inter-procedural analysis should handle this easily.
-            .Case("ziperr", true)
-            .Case("assfail", true)
-            .Case("db_error", true)
-            .Case("__assert", true)
-            .Case("__assert2", true)
-            // For the purpose of static analysis, we do not care that
-            //  this MSVC function will return if the user decides to continue.
-            .Case("_wassert", true)
-            .Case("__assert_rtn", true)
-            .Case("__assert_fail", true)
-            .Case("dtrace_assfail", true)
-            .Case("yy_fatal_error", true)
-            .Case("_XCAssertionFailureHandler", true)
-            .Case("_DTAssertionFailureHandler", true)
-            .Case("_TSAssertionFailureHandler", true)
-            .Default(false);
-    }
-  }
+ if (!BuildSinks && CE.isGlobalCFunction()) {
+   if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
+     // HACK: Some functions are not marked noreturn, and don't return.
+     //  Here are a few hardwired ones.  If this takes too long, we can
+     //  potentially cache these results.
+     //
+     // (!) In case of function list update, please also update
+     // CFGBuilder::VisitCallExpr (CFG.cpp)
+     BuildSinks =
+         llvm::StringSwitch<bool>(StringRef(II->getName()))
+             .Case("exit", true)
+             .Case("abort", true)
+             .Case("panic", true)
+             .Case("error", true)
+             .Case("Assert", true)
+             // FIXME: This is just a wrapper around throwing an exception.
+             //  Eventually inter-procedural analysis should handle this
+             //  easily.
+             .Case("ziperr", true)
+             .Case("assfail", true)
+             .Case("db_error", true)
+             .Case("__assert", true)
+             .Case("__assert2", true)
+             // For the purpose of static analysis, we do not care that
+             //  this MSVC function will return if the user decides to
+             //  continue.
+             .Case("_wassert", true)
+             .Case("__assert_rtn", true)
+             .Case("__assert_fail", true)
+             .Case("dtrace_assfail", true)
+             .Case("yy_fatal_error", true)
+             .Case("_XCAssertionFailureHandler", true)
+             .Case("_DTAssertionFailureHandler", true)
+             .Case("_TSAssertionFailureHandler", true)
+             .Case("__builtin_trap", true)
+             .Case("__builtin_unreachable", true)
+             .Default(false);
+   }
+ }
 
   if (BuildSinks)
     C.generateSink(C.getState(), C.getPredecessor());
diff --git a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
index 9fb7bebdbe41e..9150b3e155533 100644
--- a/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp
@@ -49,6 +49,7 @@ using namespace ast_matchers;
 using llvm::IsStringMapEntry;
 using ::testing::DescribeMatcher;
 using ::testing::IsEmpty;
+using ::testing::Not;
 using ::testing::NotNull;
 using ::testing::Test;
 using ::testing::UnorderedElementsAre;
@@ -693,6 +694,86 @@ TEST_F(NoreturnDestructorTest, ConditionalOperatorNestedBranchReturns) {
   // FIXME: Called functions at point `p` should contain only "foo".
 }
 
+class AnalyzerNoreturnTest : public Test {
+protected:
+  template <typename Matcher>
+  void runDataflow(llvm::StringRef Code, Matcher Expectations) {
+    tooling::FileContentMappings FilesContents;
+    FilesContents.push_back(
+        std::make_pair<std::string, std::string>("noreturn_test_defs.h", R"(
+      void assertionHandler() __attribute__((analyzer_noreturn));
+
+      void assertionTrampoline() {
+        assertionHandler();
+      }
+
+      void trap() {}
+    )"));
+
+    ASSERT_THAT_ERROR(
+        test::checkDataflow<FunctionCallAnalysis>(
+            AnalysisInputs<FunctionCallAnalysis>(
+                Code, ast_matchers::hasName("target"),
+                [](ASTContext &C, Environment &) {
+                  return FunctionCallAnalysis(C);
+                })
+                .withASTBuildArgs({"-fsyntax-only", "-std=c++17"})
+                .withASTBuildVirtualMappedFiles(std::move(FilesContents)),
+            /*VerifyResults=*/
+            [&Expectations](
+                const llvm::StringMap<
+                    DataflowAnalysisState<FunctionCallLattice>> &Results,
+                const AnalysisOutputs &) {
+              EXPECT_THAT(Results, Expectations);
+            }),
+        llvm::Succeeded());
+  }
+};
+
+TEST_F(AnalyzerNoreturnTest, Breathing) {
+  std::string Code = R"(
+    #include "noreturn_test_defs.h"
+
+    void target() {
+      trap();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, UnorderedElementsAre(IsStringMapEntry(
+                        "p", HoldsFunctionCallLattice(HasCalledFunctions(
+                                 UnorderedElementsAre("trap"))))));
+}
+
+TEST_F(AnalyzerNoreturnTest, DirectNoReturnCall) {
+  std::string Code = R"(
+    #include "noreturn_test_defs.h"
+
+    void target() {
+      assertionHandler();
+      trap();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry(
+                        "p", HoldsFunctionCallLattice(HasCalledFunctions(
+                                 UnorderedElementsAre("trap")))))));
+}
+
+TEST_F(AnalyzerNoreturnTest, IndirectNoReturnCall) {
+  std::string Code = R"(
+    #include "noreturn_test_defs.h"
+
+    void target() {
+      assertionTrampoline();
+      trap();
+      // [[p]]
+    }
+  )";
+  runDataflow(Code, Not(UnorderedElementsAre(IsStringMapEntry(
+                        "p", HoldsFunctionCallLattice(HasCalledFunctions(
+                                 UnorderedElementsAre("trap")))))));
+}
+
 // Models an analysis that uses flow conditions.
 class SpecialBoolAnalysis final
     : public DataflowAnalysis<SpecialBoolAnalysis, NoopLattice> {

Xazax-hun

I think this is a good change in general, but I wonder what is the performance impact.
Note that clang CFGs have many clients and some of them might not need this information. If there is a measurable performance cost, we might want to put this behind a flag, especially (if I remember correctly) as there are some on by default warnings that build the CFG.

Xazax-hun · 2025-07-07T11:00:55Z

clang/lib/Analysis/CFG.cpp

+
+    // Some well-known 'noreturn' functions
+    if (!NoReturn)
+      NoReturn = llvm::StringSwitch<bool>(FD->getQualifiedNameAsString())


@AaronBallman I wonder how you feel about having a list of well known (non standard library) functions hardcoded in the compiler?

I wonder if we want to have a more principled approach here long term, e.g., having one place in the compiler that injects annotations (like noreturn), and the rest of the code paths only handling that one annotation.

Xazax-hun · 2025-07-07T11:05:08Z

clang/lib/Analysis/CFG.cpp

+        return false;
+
+      auto CalleeCFG =
+          CFG::buildCFG(FD, FD->getBody(), &FD->getASTContext(), {});


Is it possible that we end up building the CFG for the same functions over and over again? This sounds like potentially wasteful.

As I understand it (please correct me if I'm wrong), FunctionDecl* instances within the constructed AST will be reused when building the CFG - if so, we could do something like:

if( FD->isAnalyzerNoReturn() || NoReturnFromCFG() ) { const_cast<FunctionDecl *>(FD)->addAttr(AnalyzerNoReturnAttr::Create( FD->getASTContext(), FD->getLocation())); return true; }

If we decide to create attrs, be sure to create them as implicit, as they were not spelled in source.

github-actions · 2025-07-07T11:17:04Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- clang-tools-extra/test/clang-tidy/checkers/bugprone/unchecked-optional-access.cpp clang/include/clang/AST/Decl.h clang/lib/AST/Decl.cpp clang/lib/Analysis/CFG.cpp clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp clang/unittests/Analysis/FlowSensitive/TransferTest.cpp clang/unittests/Analysis/FlowSensitive/TypeErasedDataflowAnalysisTest.cpp

View the diff from clang-format here.

diff --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 5dcde0052..4ec3b0f2d 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -41,8 +41,8 @@
 #include "llvm/ADT/APSInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallVector.h"
diff --git a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
index 834bd81c2..8dd46bafa 100644
--- a/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp
@@ -50,45 +50,45 @@ void NoReturnFunctionChecker::checkPostCall(const CallEvent &CE,
       BuildSinks = getFunctionExtInfo(C->getType()).getNoReturn();
   }
 
- if (!BuildSinks && CE.isGlobalCFunction()) {
-   if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
-     // HACK: Some functions are not marked noreturn, and don't return.
-     //  Here are a few hardwired ones.  If this takes too long, we can
-     //  potentially cache these results.
-     //
-     // (!) In case of function list update, please also update
-     // CFGBuilder::VisitCallExpr (CFG.cpp)
-     BuildSinks =
-         llvm::StringSwitch<bool>(StringRef(II->getName()))
-             .Case("exit", true)
-             .Case("abort", true)
-             .Case("panic", true)
-             .Case("error", true)
-             .Case("Assert", true)
-             // FIXME: This is just a wrapper around throwing an exception.
-             //  Eventually inter-procedural analysis should handle this
-             //  easily.
-             .Case("ziperr", true)
-             .Case("assfail", true)
-             .Case("db_error", true)
-             .Case("__assert", true)
-             .Case("__assert2", true)
-             // For the purpose of static analysis, we do not care that
-             //  this MSVC function will return if the user decides to
-             //  continue.
-             .Case("_wassert", true)
-             .Case("__assert_rtn", true)
-             .Case("__assert_fail", true)
-             .Case("dtrace_assfail", true)
-             .Case("yy_fatal_error", true)
-             .Case("_XCAssertionFailureHandler", true)
-             .Case("_DTAssertionFailureHandler", true)
-             .Case("_TSAssertionFailureHandler", true)
-             .Case("__builtin_trap", true)
-             .Case("__builtin_unreachable", true)
-             .Default(false);
-   }
- }
+  if (!BuildSinks && CE.isGlobalCFunction()) {
+    if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
+      // HACK: Some functions are not marked noreturn, and don't return.
+      //  Here are a few hardwired ones.  If this takes too long, we can
+      //  potentially cache these results.
+      //
+      // (!) In case of function list update, please also update
+      // CFGBuilder::VisitCallExpr (CFG.cpp)
+      BuildSinks =
+          llvm::StringSwitch<bool>(StringRef(II->getName()))
+              .Case("exit", true)
+              .Case("abort", true)
+              .Case("panic", true)
+              .Case("error", true)
+              .Case("Assert", true)
+              // FIXME: This is just a wrapper around throwing an exception.
+              //  Eventually inter-procedural analysis should handle this
+              //  easily.
+              .Case("ziperr", true)
+              .Case("assfail", true)
+              .Case("db_error", true)
+              .Case("__assert", true)
+              .Case("__assert2", true)
+              // For the purpose of static analysis, we do not care that
+              //  this MSVC function will return if the user decides to
+              //  continue.
+              .Case("_wassert", true)
+              .Case("__assert_rtn", true)
+              .Case("__assert_fail", true)
+              .Case("dtrace_assfail", true)
+              .Case("yy_fatal_error", true)
+              .Case("_XCAssertionFailureHandler", true)
+              .Case("_DTAssertionFailureHandler", true)
+              .Case("_TSAssertionFailureHandler", true)
+              .Case("__builtin_trap", true)
+              .Case("__builtin_unreachable", true)
+              .Default(false);
+    }
+  }
 
   if (BuildSinks)
     C.generateSink(C.getState(), C.getPredecessor());

steakhal

Thank you for working on this. I think its a great step forward.
The code looks good, but I found a couple issues, including a blocker.
I've not spent too much on the review, I might have missed something.

Thanks again for the PR :)

steakhal · 2025-07-07T18:21:50Z

clang/lib/StaticAnalyzer/Checkers/NoReturnFunctionChecker.cpp

+ if (!BuildSinks && CE.isGlobalCFunction()) {
+   if (const IdentifierInfo *II = CE.getCalleeIdentifier()) {
+     // HACK: Some functions are not marked noreturn, and don't return.
+     //  Here are a few hardwired ones.  If this takes too long, we can
+     //  potentially cache these results.
+     //
+     // (!) In case of function list update, please also update
+     // CFGBuilder::VisitCallExpr (CFG.cpp)
+     BuildSinks =
+         llvm::StringSwitch<bool>(StringRef(II->getName()))
+             .Case("exit", true)
+             .Case("abort", true)
+             .Case("panic", true)
+             .Case("error", true)
+             .Case("Assert", true)
+             // FIXME: This is just a wrapper around throwing an exception.
+             //  Eventually inter-procedural analysis should handle this
+             //  easily.
+             .Case("ziperr", true)
+             .Case("assfail", true)
+             .Case("db_error", true)
+             .Case("__assert", true)
+             .Case("__assert2", true)
+             // For the purpose of static analysis, we do not care that
+             //  this MSVC function will return if the user decides to
+             //  continue.
+             .Case("_wassert", true)
+             .Case("__assert_rtn", true)
+             .Case("__assert_fail", true)
+             .Case("dtrace_assfail", true)
+             .Case("yy_fatal_error", true)
+             .Case("_XCAssertionFailureHandler", true)
+             .Case("_DTAssertionFailureHandler", true)
+             .Case("_TSAssertionFailureHandler", true)
+             .Case("__builtin_trap", true)
+             .Case("__builtin_unreachable", true)
+             .Default(false);
+   }
+ }


Could the analyzer depend on the injected noreturn attributes here instead of matching again the raw identifier strings in the analyzer (again, after the CFG contruction)?

steakhal · 2025-07-07T18:23:29Z

clang/unittests/Analysis/FlowSensitive/TransferTest.cpp

+        // target() considered as 'noreturn' by CFG
+        EXPECT_TRUE(Results.keys().empty());


This was changed from ASSERT_XXX to EXPECT_XXX, which means that the test will continue to execute after this change, potentially unleashing UB. Have you checked that continuing on expectation failure is the right thing to do here?

steakhal · 2025-07-07T18:28:53Z

clang/lib/Analysis/CFG.cpp

+    if (!CE)
+      return false;
+
+    static thread_local llvm::SmallPtrSet<const FunctionDecl *, 32> InProgress;


The use of static locals is a red-flag.
Consider running gtests, where multiple clang instances will be created in sequence, slowly filling up the static cache with decl pointer that will get dangling after each test. Once in a while after a long running unittest you would get a cache hit for a dangling pointer and the disaster happens.

This is not a theoretical case. It cost me 2 days.

steakhal · 2025-07-07T18:30:45Z

clang/lib/Analysis/CFG.cpp

+      return CalleeCFG && CalleeCFG->getEntry().isInevitablySinking();
+    };
+
+    return FD->isAnalyzerNoReturn() || NoReturnFromCFG();


What if we have redecls, does it matter which has the attribute, or do we always bind the attribute to the canonical decl?

steakhal · 2025-07-07T18:31:52Z

clang/lib/Analysis/CFG.cpp

+    if (!FD || InProgress.count(FD))
+      return false;
+
+    InProgress.insert(FD);


We would do a double lookup here with InProgress. The first would be in count(FD and the second will be on insert(FD). You could use try_emplace and check if insertion happened.

Initial implementation

fc3b77d

negativ changed the title ~~Clang: analyzer_noreturn propagation in CFG~~ Clang: *noreturn propagation in CFG Jul 1, 2025

MikeWeller reviewed Jul 1, 2025

View reviewed changes

negativ added 3 commits July 4, 2025 18:25

Unittests

0b2e72d

Merge branch 'llvm:main' into analyzer_noreturn

3562ea0

Crash fix & PR fixes

06cbe1d

negativ changed the title ~~Clang: *noreturn propagation in CFG~~ [Clang]: *noreturn propagation in CFG Jul 7, 2025

negativ changed the title ~~[Clang]: *noreturn propagation in CFG~~ [clang]: *noreturn propagation in CFG Jul 7, 2025

negativ changed the title ~~[clang]: *noreturn propagation in CFG~~ [clang]: Propagate *noreturn attributes in CFG Jul 7, 2025

Merge branch 'main' into analyzer_noreturn

400dbd1

negativ marked this pull request as ready for review July 7, 2025 09:57

Xazax-hun reviewed Jul 7, 2025

View reviewed changes

Fix some test failures

918475d

steakhal requested changes Jul 7, 2025

View reviewed changes

		// target() considered as 'noreturn' by CFG
		EXPECT_TRUE(Results.keys().empty());

[clang]: Propagate *noreturn attributes in CFG #146355

Are you sure you want to change the base?

[clang]: Propagate *noreturn attributes in CFG #146355

Conversation

negativ commented Jun 30, 2025

Summary

Problem

Solution

Implementation Details

Uh oh!

github-actions bot commented Jun 30, 2025

Uh oh!

MikeWeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llvmbot commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Implementation Details

Uh oh!

llvmbot commented Jul 7, 2025

Summary

Problem

Solution

Implementation Details

Uh oh!

Xazax-hun left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steakhal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

[clang]: Propagate `*noreturn` attributes in `CFG` #146355

[clang]: Propagate `*noreturn` attributes in `CFG` #146355

llvmbot commented Jul 7, 2025 •

edited

Loading

Xazax-hun left a comment •

edited

Loading

github-actions bot commented Jul 7, 2025 •

edited

Loading